Replace custom EnvVar with corev1.EnvVar by anveshthakur · Pull Request #4570 · stacklok/toolhive

anveshthakur · 2026-04-06T08:04:15Z

Summary

Fixes #
#4547

Type of change

Test plan

Unit tests (task test)
E2E tests (task test-e2e)
Linting (task lint-fix)
Manual testing (describe below)

Changes

File	Change

Does this introduce a user-facing change?

Special notes for reviewers

jerm-dro

Thanks for the PR! The type migration is clean and the test updates are thorough.

question: EmbeddingServerSpec.Env (at embeddingserver_types.go:76) still uses the custom EnvVar type, which prevents deleting it as #4547 suggests. Should it be included in this PR?

jerm-dro · 2026-04-06T18:19:29Z

cmd/thv-operator/controllers/mcpserver_runconfig.go


 // convertEnvVarsFromMCPServer converts MCPServer environment variables to builder format
-func convertEnvVarsFromMCPServer(envs []mcpv1alpha1.EnvVar) map[string]string {
+func convertEnvVarsFromMCPServer(envs []corev1.EnvVar) map[string]string {


blocker: The issue (#4547) notes that controller conversion logic should be simplified now that the types match directly. This function converts to map[string]string which is fine for the RunConfig path (JSON config file). But the same Name/Value-only copy pattern exists at mcpserver_controller.go:1142 and :1734, where it builds a new corev1.EnvVar{Name: envVar.Name, Value: envVar.Value} from what is already a corev1.EnvVar — silently dropping ValueFrom when setting env vars on the proxy Deployment pod spec.

Those two locations should be simplified to:

env = append(env, m.Spec.ResourceOverrides.ProxyDeployment.Env...)

Without this, valueFrom references on proxy env vars won't propagate to the pod spec.

Good Catch ! Totally missed these
Made changes for this.

@jerm-dro

anveshthakur · 2026-04-07T04:57:33Z

Thanks for the PR! The type migration is clean and the test updates are thorough.

question: EmbeddingServerSpec.Env (at embeddingserver_types.go:76) still uses the custom EnvVar type, which prevents deleting it as #4547 suggests. Should it be included in this PR?

#4570 didn't mention this location
I can migrate this as well to this type @jerm-dro just LMK

…into issue-4547

jerm-dro · 2026-04-09T22:05:00Z

Yes please, include EmbeddingServerSpec.Env in this PR so the custom EnvVar type can be fully removed.

codecov · 2026-04-09T22:09:33Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.65%. Comparing base (b542727) to head (8dcfd7b).
⚠️ Report is 22 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4570      +/-   ##
==========================================
+ Coverage   68.60%   68.65%   +0.04%     
==========================================
  Files         515      515              
  Lines       53518    53510       -8     
==========================================
+ Hits        36718    36735      +17     
+ Misses      13958    13928      -30     
- Partials     2842     2847       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…e-4547

…into issue-4547

anveshthakur · 2026-04-11T20:48:21Z

Yes please, include EmbeddingServerSpec.Env in this PR so the custom EnvVar type can be fully removed.

Heu @jerm-dro, made changes for these as well.

ChrisJBurns

Multi-Agent Consensus Review

Agents consulted: kubernetes-expert, code-reviewer, toolhive-expert, go-security-reviewer

Consensus Summary

#	Finding	Consensus	Severity	Action
1	CRD YAML manifests not regenerated	10/10	HIGH	Fix
2	`convertEnvVarsFromMCPServer` silently drops `ValueFrom`	10/10	HIGH	Fix
3	EmbeddingServer controller also drops `ValueFrom`	9/10	HIGH	Fix
4	Custom EnvVar type commented out, not deleted	9/10	MEDIUM	Fix
5	No test coverage for `ValueFrom` scenarios	7/10	MEDIUM	Fix
6	Direct pass-through expands attack surface (FieldRef)	7/10	MEDIUM	Discuss
7	Deep copy import alias inconsistency	7/10	LOW	Verify

Overall

The type migration from custom EnvVar to corev1.EnvVar is the right architectural direction — it eliminates a parallel type that was drifting from the Kubernetes standard and enables proper ValueFrom support for secret/configmap references. The mechanical replacement across 22 files is clean, consistent, and thorough.

However, the PR has a critical completeness gap: it changes the Go types but does not regenerate the CRD YAML manifests (task operator-manifests was not run), so the API server schema still enforces the old required: [name, value] and lacks the valueFrom field entirely. More importantly, two code paths — convertEnvVarsFromMCPServer in the RunConfig path and buildEnvVars in the EmbeddingServer controller — silently drop ValueFrom references, producing empty strings instead of the intended secret values. This creates a dangerous gap between what the CRD schema accepts and what the runtime actually handles.

The findings are blocking but straightforward to fix: regenerate CRD manifests, delete the commented-out dead code, and either reject ValueFrom with a clear error where it can't be supported (RunConfig path) or pass it through where it can (EmbeddingServer controller, which already builds []corev1.EnvVar).

Conclusion

Due to us getting v1beta1 in the next couple of days, I think it's probably wise to push this out to v1beta2. This will mean leaving this PR up for a while longer. If that's ok with you @anveshthakur then feel free to address the comments below, or you can close and keep the issue in your name and we'll slot it in nearing to v1beta2, what do you think?

Findings not attached inline

[HIGH] F1: CRD YAML manifests not regenerated (Consensus: 10/10)
The Go types changed from custom EnvVar to corev1.EnvVar but no CRD YAML files under deploy/charts/operator-crds/files/crds/ appear in this PR. Per project conventions, task operator-manifests must be run after modifying CRD types. Without this, the API server schema still has required: [name, value] and lacks the valueFrom field — the type change has no effect on cluster validation. (Raised by: kubernetes-expert, code-reviewer, toolhive-expert)

[MEDIUM] F5: No test coverage for ValueFrom scenarios (Consensus: 7/10)
All test updates use corev1.EnvVar{Name: "...", Value: "..."} — the same pattern as the old custom type. No tests verify behavior when ValueFrom is set. Add tests that confirm convertEnvVarsFromMCPServer rejects/warns on ValueFrom entries (once validation is added), and tests confirming the Deployment path properly propagates ValueFrom. (Raised by: code-reviewer, go-security-reviewer)

Generated with Claude Code

ChrisJBurns · 2026-04-13T15:03:03Z

cmd/thv-operator/api/v1alpha1/mcpserver_types.go

 // EnvVar represents an environment variable in a container
-type EnvVar struct {
-	// Name of the environment variable
-	// +kubebuilder:validation:Required
-	Name string `json:"name"`
-
-	// Value of the environment variable
-	// +kubebuilder:validation:Required
-	Value string `json:"value"`
-}
+// type EnvVar struct {
+// 	// Name of the environment variable
+// 	// +kubebuilder:validation:Required
+// 	Name string `json:"name"`
+
+// 	// Value of the environment variable
+// 	// +kubebuilder:validation:Required
+// 	Value string `json:"value"`
+// }


[MEDIUM] F4: Custom EnvVar type commented out, not deleted (Consensus: 9/10)

Dead commented-out code should be deleted entirely. Git history preserves the original type definition if it's ever needed for reference. The zz_generated.deepcopy.go already removed the EnvVar.DeepCopyInto and EnvVar.DeepCopy methods, confirming the type is no longer used anywhere.

Delete this entire block (lines 431-440).

Raised by: kubernetes-expert, code-reviewer, toolhive-expert

ChrisJBurns · 2026-04-13T15:03:03Z

cmd/thv-operator/controllers/mcpserver_runconfig.go


 // convertEnvVarsFromMCPServer converts MCPServer environment variables to builder format
-func convertEnvVarsFromMCPServer(envs []mcpv1alpha1.EnvVar) map[string]string {
+func convertEnvVarsFromMCPServer(envs []corev1.EnvVar) map[string]string {


[HIGH] F2: convertEnvVarsFromMCPServer silently drops ValueFrom (Consensus: 10/10)

This function now accepts []corev1.EnvVar but only reads .Name and .Value. If a user specifies valueFrom: secretKeyRef: ..., the value will silently be set to an empty string. The downstream runner.RunConfig uses map[string]string and cannot support ValueFrom references.

Add validation that detects and rejects ValueFrom entries with a clear error rather than silently dropping the data.

Raised by: kubernetes-expert, code-reviewer, toolhive-expert, go-security-reviewer

ChrisJBurns · 2026-04-13T15:03:03Z

cmd/thv-operator/controllers/mcpserver_controller.go

-				Value: envVar.Value,
-			})
-		}
+		env = append(env, m.Spec.ResourceOverrides.ProxyDeployment.Env...)


[MEDIUM] F6: Direct pass-through now includes FieldRef/ResourceFieldRef (Consensus: 7/10)

Nice simplification, but the full corev1.EnvVar type also includes ValueFrom.FieldRef and ValueFrom.ResourceFieldRef, which can leak node/pod metadata (hostIP, nodeName, etc.) to MCP server containers. Consider whether this expanded attack surface is acceptable, or whether validation should restrict ValueFrom to only SecretKeyRef and ConfigMapKeyRef.

Raised by: go-security-reviewer

ChrisJBurns · 2026-04-13T15:03:03Z

cmd/thv-operator/api/v1alpha1/embeddingserver_types.go

 	// +listMapKey=name
 	// +optional
-	Env []EnvVar `json:"env,omitempty"`
+	Env []corev1.EnvVar `json:"env,omitempty"`


[HIGH] F3: EmbeddingServer controller buildEnvVars also drops ValueFrom (Consensus: 9/10)

The EmbeddingServerReconciler.buildEnvVars function at embeddingserver_controller.go:640-644 manually copies only Name/Value into new corev1.EnvVar structs, discarding ValueFrom. Since this function already returns []corev1.EnvVar, the fix is straightforward: replace the conversion loop with envVars = append(envVars, embedding.Spec.Env...) to pass through the full struct, matching the pattern used in the MCPServer proxy deployment code.

Raised by: kubernetes-expert, code-reviewer

ChrisJBurns · 2026-04-13T15:03:03Z

cmd/thv-operator/api/v1alpha1/zz_generated.deepcopy.go


 import (
-	corev1 "k8s.io/api/core/v1"
+	"k8s.io/api/core/v1"


[LOW] F7: Generated deepcopy import alias changed from corev1 to bare v1 (Consensus: 7/10)

The import alias for k8s.io/api/core/v1 changed from the conventional corev1 to bare v1. This is generated code so it compiles fine, but suggests a different controller-gen version was used. Verify by running task operator-generate and confirming the output matches what's committed here.

Raised by: kubernetes-expert, code-reviewer, toolhive-expert

Replace custom EnvVar with corev1.EnvVar

5cc6965

anveshthakur requested review from ChrisJBurns, JAORMX, jerm-dro, jhrozek and yrobla as code owners April 6, 2026 08:04

Merge branch 'main' into issue-4547

cfac2af

jerm-dro requested changes Apr 6, 2026

View reviewed changes

Merge branch 'main' into issue-4547

345f1ca

anveshthakur and others added 5 commits April 7, 2026 10:44

updates controller code

b29cd5e

Merge branch 'main' into issue-4547

ca9dc98

Replace custom EnvVar with corev1.EnvVar

7a502e8

updates controller code

c1388bc

Merge branch 'issue-4547' of https://github.com/anveshthakur/toolhive …

6b4cd67

…into issue-4547

anveshthakur requested review from amirejaz, blkt, lujunsan and rdimitrov as code owners April 9, 2026 07:42

Merge branch 'main' into issue-4547

8dcfd7b

github-actions bot added the size/S Small PR: 100-299 lines changed label Apr 9, 2026

anveshthakur added 4 commits April 12, 2026 01:23

Merge branch 'main' of https://github.com/stacklok/toolhive into issu…

82fd867

…e-4547

Merge branch 'issue-4547' of https://github.com/anveshthakur/toolhive …

0486ace

…into issue-4547

changes implementation to corev1.EnvVar

d37b279

removes EnvVar completely

b42ab61

ChrisJBurns requested changes Apr 13, 2026

View reviewed changes

Conversation

anveshthakur commented Apr 6, 2026

Summary

Type of change

Test plan

Changes

Does this introduce a user-facing change?

Special notes for reviewers

Uh oh!

jerm-dro left a comment

Choose a reason for hiding this comment

Uh oh!

jerm-dro Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

anveshthakur Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

anveshthakur commented Apr 7, 2026

Uh oh!

jerm-dro commented Apr 9, 2026

Uh oh!

codecov bot commented Apr 9, 2026

Codecov Report

Uh oh!

anveshthakur commented Apr 11, 2026

Uh oh!

ChrisJBurns left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Multi-Agent Consensus Review

Consensus Summary

Overall

Conclusion

Findings not attached inline

Uh oh!

ChrisJBurns Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

ChrisJBurns Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

ChrisJBurns Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

ChrisJBurns Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

ChrisJBurns Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ChrisJBurns left a comment •

edited

Loading